feat(snapshots): Add local image diff command using odiff#3306
Conversation
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
NicoHinderling
force-pushed
the
feat/snapshots-diff
branch
from
9875c7d to
854593b
Compare
|
Seems to be working quite well re local testing! 🥳 |
cameroncooke
left a comment
cameroncooke
left a comment
There was a problem hiding this comment.
Looks good to me, I can't comment on the Rust code but it looks clean and well designed. I've also done some testing and it's rock solid in my testing.
NicoHinderling
force-pushed
the
feat/snapshots-diff
branch
from
41af7fd to
0d88866
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
NicoHinderling
force-pushed
the
feat/snapshots-diff
branch
from
1c7b0cc to
3ce0749
Compare
NicoHinderling
force-pushed
the
feat/snapshots-diff
branch
from
3ce0749 to
999c951
Compare
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
I have some concerns with how we are installing odiff (we already chatted on Slack, but there are more details here). Also, the API feels a bit unintuitive, with the snapshots diff command being on a separate subcommand from the existing build snapshots upload functionality.
Please see the relevant inline comments for more details.
There was a problem hiding this comment.
m: Please add the Emerge Tools team as CODEOWNERS of src/commands/snapshots, either in this PR, or a follow up. That way, you will not be blocked on my reviews for further changes here.
There was a problem hiding this comment.
h: I am concerned about this runtime installer/cache approach.
My main concern is that it is very strange of us to be pulling in an executable binary, within a directory we manage, rather than doing this via a package manager or normal build-time dependency. I expect this will be difficult to maintain long term.
Some other minor things:
- Old versions of
odiffappear to remain indefinitely in the cache. odiffessentially becomes an undocumented dependency, which users will not be aware of.- The cache also introduces avoidable TOCTOU/permissions concerns around trusting and executing a mutable cached binary.
- There is also a concrete mismatch on Windows: this expects
odiff-win-x64.exe, butodiff-bin@4.3.8shipsodiff-windows-x64.exe.
Possible alternatives
The ideal alternative would be to use a library for this, rather than relying on an external binary. Unfortunately, however, odiff does not expose a Rust API, and the Rust-native image diff crates I could find (blazediff and image-compare) appear to be less widely used than odiff, so I am not sure how well they would be maintained.
Therefore, it probably still makes sense to use the odiff binary; I'd just treat it as an external dependency rather than trying to manage it ourselves.
Basically, when running commands which require odiff, we should check whether it is available on PATH and whether odiff --version is supported (I'd make the version check a soft-validation so we only warn, but don't error, if the version is not in our officially supported range). If odiff is not installed, or fails to run after detecting an incompatible version, we should error and tell the user they need to install odiff. Or, even better, we could offer to install odiff for them using npm, only with an explicit user prompt and opt-in, though.
Another option could be to write and maintain Rust <> odiff bindings ourselves, but I doubt that is worth the maintenance effort.
There was a problem hiding this comment.
alright, went ahead and re-implemented it
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e27edf6. Configure here.
NicoHinderling
force-pushed
the
feat/snapshots-diff
branch
2 times, most recently
from
12d4aa9 to
cf94ea1
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
NicoHinderling
force-pushed
the
feat/snapshots-diff
branch
from
cf94ea1 to
a70417b
Compare
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
lgtm with some minor (all optional and low-severity) comments; thank you for addressing my previous feedback!
Note that I mainly checked the overall structure, not the specifics of how the diff logic is implemented, as I think someone on the Emerge team would have more context there
| which::which("odiff").context( | ||
| "odiff was installed but could not be found on PATH. \ | ||
| You may need to restart your shell.", | ||
| ) |
There was a problem hiding this comment.
l: Only if possible and easy to do, I would suggest that we print the path to where the odiff binary was installed in this error message, to help users debug this failure mode.
I am guessing it will be a pretty rare failure mode, so if non-trivial to implement, let's skip it.
Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>
Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>
When odiff is installed via npm but not found on PATH, include the npm global prefix in the error message to help users debug the issue. Co-Authored-By: Claude <noreply@anthropic.com>
#17903) ## DESCRIBE YOUR PR Documents the new `sentry-cli snapshots download` and `sentry-cli snapshots diff` commands added in [sentry-cli#3306](getsentry/sentry-cli#3306) and [sentry-cli#3310](getsentry/sentry-cli#3310). - New "Local Testing" product docs page covering the workflow: download baselines → run tests → diff locally - CLI reference sections for `snapshots download` and `snapshots diff` with flag tables ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): - [x] Other deadline: - [ ] None: Not urgent, can wait up to 1 week+ ## SLA - Teamwork makes the dream work, so please add a reviewer to your PRs. - Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it. Thanks in advance for your help! ## PRE-MERGE CHECKLIST *Make sure you've checked the following before merging your changes:* - [ ] Checked Vercel preview for correctness, including links - [ ] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs) --------- Co-authored-by: Claude <noreply@anthropic.com>
3 participants
Summary
sentry-cli snapshots diff <base_dir> <head_dir>for locally comparing directories of PNG snapshot images using odiff~/.sentry-cli/odiff/<version>/odiff--servermode for efficient batch image comparison over stdin/stdout JSON protocol--threshold,--no-antialiasing,--fail-on-diff,--selective, and--outputflagsSelective mode
Supports
--selectiveflag matching the backend's simple selective diff mode. When set, images present in the base directory but missing from the head directory are categorized asskippedinstead ofremoved. This is intended for partial test runs where only a subset of screenshots are captured — missing images are not treated as deletions.Security
Config)Motivation
Enables AI agents (and developers) to rapidly validate visual changes by diffing screenshots/snapshots locally without needing server-side infrastructure. This pairs with
sentry-mcpfor end-to-end snapshot test investigation workflows.Example
$ sentry-cli snapshots diff ./base-snapshots ./head-snapshots --fail-on-diff { "summary": { "total": 6, "changed": 6, "unchanged": 0, "added": 0, "removed": 0, "skipped": 0, "errored": 0 }, "images": [ { "name": "sidebar-dark.png", "status": "changed", "diff_percentage": 4.18, "diff_pixel_count": 30442, "diff_mask_path": "./diff-output/sidebar-dark.png" }, ... ] }# Selective mode: missing base images are "skipped", not "removed" $ sentry-cli snapshots diff ./base-snapshots ./partial-head-snapshots --selectiveTest plan
cargo test snapshots— help output and missing-dir error trycmd tests passcargo check --features managed— compiles with managed feature flag--fail-on-diffreturns exit code 1 when diffs found--selectivecorrectly categorizes base-only images asskipped🤖 Generated with Claude Code